Skip to content

fix: don't revert claim_rewards on un-convertible dust#1474

Merged
enthusiastmartin merged 3 commits into
masterfrom
fix/referrals-claim-dust-revert
Jun 9, 2026
Merged

fix: don't revert claim_rewards on un-convertible dust#1474
enthusiastmartin merged 3 commits into
masterfrom
fix/referrals-claim-dust-revert

Conversation

@mrq1911

@mrq1911 mrq1911 commented Jun 9, 2026

Copy link
Copy Markdown
Member

problem

claim_rewards iterates the global PendingConversions map and converts each pending asset to the reward asset before paying out shares. it only tolerates two conversion errors — ConversionMinTradingAmountNotReached and ConversionZeroAmountReceived — and reverts the whole call on anything else.

the v49 fee-flow refactor moved conversion to ConvertViaOmnipool, which dropped the old explicit min-amount pre-check (that returned ConversionMinTradingAmountNotReached). it now calls Omnipool::sell directly → for a pot balance below MinTradingLimit (1_000 raw units) it propagates the raw pallet_omnipool::Error::InsufficientTradingAmount, which is not in the tolerated set.

result → any sub-MinTradingLimit dust balance of a non-reward asset in PendingConversions makes claim_rewards revert:

  • since extrinsics run before on_idle, a dust entry left by a trade earlier in the same block blocks every claim_rewards in that block
  • weaponizable as a cheap, permissionless same-block front-run grief against reward claimers (self-link + tiny linked trade tuned so the 5% slice lands in [1, 999] of an obscure asset)
  • also causes incidental spurious claim failures for honest users
  • no funds lost — rewards stay claimable next block — but it's a real correctness/griefing defect

regression vs v48: the old convert returned ConversionMinTradingAmountNotReached for sub-min amounts (which the claim loop tolerated); the new converter returns InsufficientTradingAmount, and the claim loop's tolerated-error list was never updated.

fix

make the claim loop best-effort, identical to the sibling on_idle hook (which already does let _ = T::Convert::convert(...)): skip an un-convertible slice instead of reverting, and drop the entry either way. a skipped conversion now emits a ConversionFailed { asset_id, reason } event (in both claim_rewards and on_idle) so it is surfaced rather than silently swallowed — mirroring the fee-processor.

  • the conversion is genuinely best-effort → funds stay in the pot, reward accounting is share-based and independent of the swap, and the asset re-queues on the next fee
  • removes the claim_rewardson_idle tolerance divergence by construction

why not extend the exact-match list instead?

the alternative is to keep the if let Err propagation and add the new tolerated variants (InsufficientTradingAmount, ConversionFailed, PriceNotAvailable). rejected because:

  • it's what caused this bug. the tolerated list was coupled to the converter's exact error variants; when the converter impl changed, the list silently went stale and started reverting. any new list carries the same drift risk on the next converter change.
  • it can't be written cleanly here. pallet-referrals constrains its converter to Convert<…, Error = DispatchError> and does not depend on pallet-omnipool/pallet-fee-processor. matching their error variants would mean either adding those deps (bad layering for a generic rewards pallet) or matching raw DispatchError::Module discriminants (fragile).
  • the swallow-all matches on_idle, which already abandoned exact-match — so both cleanup paths now behave identically.

the visibility that exact-match gave (surfacing an unexpected converter error rather than dropping it) is preserved by the ConversionFailed event, without re-introducing the coupling.

test

claim_rewards_should_succeed_when_pending_asset_balance_is_below_min_trading_limit → a 500-unit (sub-min) DAI dust entry no longer blocks an unrelated user's claim, and a ConversionFailed event is emitted for it. the mock AssetConvert now models the production min-trading-limit rejection (returns a non-referrals error for sub-min amounts), gated behind a new with_convert_min_amount builder defaulting to disabled so existing tests are unaffected.

  • red before the one-line fix, green after; all 60 referrals tests pass

versions

  • pallet-referrals 1.6.0 → 1.6.1
  • runtime spec_version 426 → 427, hydradx-runtime 426.0.0 → 427.0.0

Copilot AI review requested due to automatic review settings June 9, 2026 20:50
@mrq1911 mrq1911 requested a review from enthusiastmartin June 9, 2026 20:51
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

Crate versions that have been updated:

  • pallet-referrals: v1.6.0 -> v1.6.1
  • hydradx-runtime: v426.0.0 -> v427.0.0

Runtime version has been increased.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a referrals runtime regression where claim_rewards could revert if PendingConversions contains a dust (sub-MinTradingLimit) balance that the new omnipool-backed converter rejects, enabling same-block griefing and incidental claim failures.

Changes:

  • Make claim_rewards conversion loop best-effort (ignore conversion errors) and always drop PendingConversions entries, matching on_idle.
  • Add a regression test for sub-min pending dust not blocking unrelated claims, and extend the mock AssetConvert to optionally reject sub-min amounts with a non-referrals error.
  • Bump pallet-referrals and runtime spec/runtime crate versions.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
runtime/hydradx/src/lib.rs Bumps spec_version to reflect runtime behavior change.
runtime/hydradx/Cargo.toml Bumps hydradx-runtime crate version.
pallets/referrals/src/lib.rs Changes claim_rewards pending conversion handling to best-effort (non-fatal).
pallets/referrals/src/tests/claim.rs Adds regression test for dust pending conversion not blocking claims.
pallets/referrals/src/tests.rs Extends test converter with optional min-amount rejection gate + builder hook.
pallets/referrals/Cargo.toml Bumps pallet-referrals crate version.
Cargo.lock Updates locked versions for the bumped crates.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pallets/referrals/src/tests/claim.rs Outdated
Comment on lines +77 to +81
// Regression: a sub-`MinTradingLimit` dust balance of a non-reward asset in `PendingConversions`
// must not block reward claims. The converter rejects the dust with a non-referrals error
// (`InsufficientTradingAmount`), which the claim loop only tolerates for its own two legacy
// variants — so today it reverts the whole claim instead of skipping the dust like `on_idle`
// does. This test asserts the desired self-healing behaviour and currently FAILS, proving the bug.
Comment on lines +92 to +94
// Price present so the converter reaches the min-amount check (not the no-price branch,
// whose error the claim loop happens to tolerate).
.with_conversion_price((HDX, DAI), EmaPrice::new(1_000_000_000_000, 1_000_000_000_000_000_000))
@mrq1911 mrq1911 changed the title fix(referrals): don't revert claim_rewards on un-convertible dust fix: don't revert claim_rewards on un-convertible dust Jun 9, 2026
@enthusiastmartin enthusiastmartin merged commit 3670a47 into master Jun 9, 2026
11 of 12 checks passed
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

Quick benchmark at commit 9bed6c4 has been executed successfully.
View results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants